Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix optional arguments in TypeScript #1483

Merged
merged 3 commits into from
May 14, 2019

Conversation

dbrgn
Copy link
Contributor

@dbrgn dbrgn commented Apr 25, 2019

For a function like this:

#[wasm_bindgen]
pub fn opt_fn(_a: Option<i32>) -> Option<i32> {
    None
}

...the currently generated typescript looks like this:

/**
* @param {number | undefined} _a
* @returns {number | undefined}
*/
export function opt_fn(_a: number | undefined): number | undefined;

Support for undefined in the parameter type was added in #1201. However, while type number | undefined is technically optional at runtime, typechecking tools will still consider the parameter to be required. (CC @rhysd, please correct me if this information is actually incorrect.)

Instead, the TypeScript should look like this, with a ? appended to the parameter name:

/**
* @param {number | undefined} _a
* @returns {number | undefined}
*/
export function opt_fn(_a?: number): number | undefined;

Reference: https://www.typescriptlang.org/docs/handbook/functions.html#optional-and-default-parameters

This PR should actually fix #1129.

Some notes: I also adjusted crates/typescript-tests/src/opt_args_and_ret.ts, but I could not find out whether and where this file is actually used. I think it's dead code, but I might be wrong.

@dbrgn
Copy link
Contributor Author

dbrgn commented Apr 25, 2019

It is used for local and CI tests.

I assumed so, but when I run those tests (with run.sh) with bad TS type declarations, nothing fails...

Copy link
Contributor

@c410-f3r c410-f3r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

Some notes: I also adjusted crates/typescript-tests/src/opt_args_and_ret.ts, but I could not find out whether and where this file is actually used. I think it's dead code, but I might be wrong.

It is used for local and CI tests.

fn optional(name: String, type_: String) -> Self {
Self { optional: true, name, type_ }
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the names of these methods be changed to new_(required|optional)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any guidelines / recommendations on this topic? I couldn't find anything in https://github.com/rust-dev-tools/fmt-rfcs/.

I personally prefer the shorter version, it's clear from the signature that it's a constructor.

@c410-f3r
Copy link
Contributor

c410-f3r commented Apr 25, 2019

I assumed so, but when I run those tests (with run.sh) with bad TS type declarations, nothing fails...

Maybe because optional arguments are automatically coerced into undefined? For example, this and this have the same output.

crates/cli-support/src/js/js2rust.rs Outdated Show resolved Hide resolved
@alexcrichton
Copy link
Contributor

Thanks for this @dbrgn! In addition to what @rhysd is mentioning, could the test in in crates/typescript-tests be updated with various flavors of optional arguments to make sure this works? (e.g. some methods, some cases about mixed optional arguments, etc)

@dbrgn dbrgn force-pushed the typescript-optional-args branch from 671f193 to 608a819 Compare May 9, 2019 16:36
@dbrgn
Copy link
Contributor Author

dbrgn commented May 9, 2019

PR updated!

@alexcrichton I added some tests, but note that the tests are a bit flaky with regards to ensuring correct output for optional and/or omittable parameters. I found several combinations that pass the test but are not actually correct. For example, this works:

// typescript_tests.d.ts
export function opt_fn_mixed(_a: number | undefined, _b: number, _c?: number): number | undefined;

// opt_args_and_ret.ts
const opt_fn_mixed: (a: number, b: number, c: number) => number | undefined = wbg.opt_fn_mixed;

The reason is that number is a type subset of number | undefined, and narrowing the set of allowed types is always possible.

I think a test function or macro that asserts the correctly generated TS output string/file would be the better approach than using TypeScript's type system for verification. But that's out of scope for this PR.

@alexcrichton
Copy link
Contributor

Ok no worries, this looks good to me! Sorry for the delay in reviewing :(

I'm fine testing more in further PRs as well, this certainly looks good enough for now! @dbrgn want to open a follow-up issue with your thoughts on testing?

@alexcrichton alexcrichton merged commit 827810f into rustwasm:master May 14, 2019
@dbrgn dbrgn deleted the typescript-optional-args branch May 14, 2019 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for named and optional parameters in .d.ts
4 participants